fix: count tracked files for test-file metric in /retro and /ship#2308
Open
joshRpowell wants to merge 1 commit into
Open
fix: count tracked files for test-file metric in /retro and /ship#2308joshRpowell wants to merge 1 commit into
joshRpowell wants to merge 1 commit into
Conversation
The test-file count scanned the whole working tree with `find`, so it
counted untracked build output as test files. On a Rails repo using
`bundle install --path vendor/bundle` it over-reported by 37x:
find . -name '*.test.*' ... | grep -v node_modules | wc -l -> 623
git ls-files | grep -E '(\.test\.|...)' | wc -l -> 17
The 623 breaks down as 433 from vendor/bundle gem suites, 189 from
.claude/worktrees/ agent copies, 4 from nested node_modules, and 17
real files under test/.
`grep -v node_modules` only filters output lines, so find still walks
every ignored tree, and it does nothing for vendor/, worktrees, tmp/,
target/, .venv/, Pods/ and so on. No exclusion list can stay complete,
so count tracked files instead: untracked build output is excluded by
definition, .gitignore is respected for free, and it avoids the
full-tree walk. Both skills already require a git repo and run
`git log origin/<default>` in the same step.
Fixes garrytan#2307
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2307.
Problem
The test-file count used by
/retroand/shipscans the working tree withfind, so untracked build output is counted as test files. On a Rails repo with bundled gems it over-reports by 37x.Measured on a Rails 8 app with 17 real test files:
/retroprints this as "Total test files: N", so the retro is just wrong./shipuses it for a before/after delta, so the delta survives but both endpoints are inflated.Two things are broken:
grep -v node_modulesfilters output lines rather than pruning the walk (find still descends into every ignored tree), and no exclusion list can be complete —vendor/is Ruby,.claude/worktrees/is gstack's own feature, and every ecosystem adds more (target/,.venv/,Pods/).Fix
Untracked build output is excluded by definition,
.gitignoreis respected for free, and the full-tree walk goes away. Both skills already require a git repo and rungit log origin/<default>in the same step, so no new dependency..claude/worktrees/is untracked in the parent repo, so this also fixes the worktree inflation without special-casing gstack's own directory.Changes
Sources of truth:
retro/SKILL.md.tmplship/sections/test-coverage.md(x2)scripts/resolvers/testing.ts(x2, inside a template literal so the escape is\\.)openclaw/skills/gstack-openclaw-retro/SKILL.mdRegenerated / updated:
retro/SKILL.mdviabun run gen:skill-docstest/fixtures/golden/{codex,factory}-ship-SKILL.mdto match the regenerated.agents/and.factory/ship skillsVerification
Generated output is correct shell (single backslashes survive the TS template literal):
Behavior on the affected repo: 623 -> 17.
Test suite (
bun test test/with the e2e/eval ignores frompackage.json):test/host-config.test.ts— 77/77 pass, including all three golden-file regression testsmain(a325940) with zero changes, so they are pre-existing and unrelated:gstack-model-benchmark --dry-run > NOT READY path fires when auth env vars are strippedsession-runner observability > 11: all new I/O is wrapped in try/catch (non-fatal)Left alone deliberately
test/fixtures/golden-ship-claude.mdstill contains the old command at lines 1090 and 1250. Nothing reads it — no.tsin the repo references that path, and it is not the fixture used by thegolden-file regressionsuite (that one istest/fixtures/golden/claude-ship-SKILL.md, which does not contain the pattern at all). It looks like an orphaned fixture from an earlier layout, so hand-editing it seemed more likely to entrench drift than fix it. Happy to update or delete it if you'd prefer.Tradeoff worth a look
Test files written but not yet
git add-ed stop counting. Correct for/retro, which reports on merged history. For/ship's before/after delta it is a slight semantic change — if you want untracked-but-not-ignored files included,git ls-files --cached --others --exclude-standarddoes that while still honoring.gitignore. Happy to switch/shipto that variant.